-
-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Minimal infrastructure for http, httpApiKey, and oauth2 security schemes #120
Conversation
e081599
to
5e44d44
Compare
Codecov Report
@@ Coverage Diff @@
## main #120 +/- ##
==========================================
+ Coverage 96.58% 97.06% +0.47%
==========================================
Files 9 11 +2
Lines 527 885 +358
==========================================
+ Hits 509 859 +350
- Misses 18 26 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
8b92fc8
to
d02b1a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very much appreciate the time and effort you've put into this PR! Thank you! Don't let the number of comments/suggestions mislead you -- it generally looks impressively sensible.
The logic and functionality looks good. Most of my comments are revolving the package/module structure. Apologies if I seem too opinionated, but I'm keen to maintain the existing style of the code as well as the structure, at least in the short term. Happy to discuss the restructuring of the code/project, but I'd like this to be part of another PR/issue. Let's keep this PR free of refactoring noise, just focusing on adding the security feature.
TL;DR of my comments:
- Let's add the new security types under the existing top-lvl
types
module. - No need for any smart scheme resolution. We can just pass the
components
section of the spec to the security handler factory. - Validation of the YAML data using the
__post_init__
dataclass method. - No need for a new security package within the asynction package. We can just have a
security.py
top-lvl module which includes the validation stuff. - Finally, the security wrapper you implemented should also be added in the mock server class.
Co-authored-by: Dimitrios Dedoussis <dimitrios@dedouss.is>
Distribute files from security/ to the corresponding existing files.
At this point I could probably use some recommendations from you on what kind of tests you'd like to see in the unit tests in order to get the code coverage up to where it needs to be. I will think on it as well, but some recommendations would be very helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing a large amount of comments from the previous review!
Added some more, so that we can take the implementation part of this PR to a solid state.
Have not yet reviewed the testing part of this PR thoroughly. The tests you've already added look sensible. However, I'd prefer to focus on testing on the next round of review, once we have got the implementation part agreed.
When addressing the comments of this round of reviews, I would suggest to start with the comments under types.py
. These include suggestions with regards to:
- Completely aligning the dataclass types with the AsyncAPI YAML data structure
- Removing redundant validation
- Adding missing validation in the correct dataclasses
With regards to the deserialisation validation of types.py
:
- The
AsyncAPISpec
class should validate the theservers.securiy_requirements
adhere to thecomponents.security_schemes
definitions. - The
SecurityScheme
class should validate that all the necessary dataclass fields are initialised based on the scheme type. - The
OAuthFlows
class should validate that each of the flows has all the fields required. For example theinternal
flow should include anauthorization_url
field etc.
Once you address the types.py
comments, you would have the foundation needed to address the rest of the comments.
I also noticed that someone has already proposed adding more granular level security asyncapi/spec#584 (review) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the late review. Implementation looks good! Added some extra comments with regards to typing annotations.
With respect to testing:
- The integration tests you've added look very good.
- An e2e test would also be needed,
- There are plenty of unit tests missing. For example, I'd like to see tests in tests_types.py covering the
__post_init__
validators, as well as making sure that the security schemes that interest us can be deserialised (only the basic http scheme is checked atm). Within test_server.py, you will also need to add tests that make sure that the correct security checkers wrap the registered handlers. The same logic as the tests of the validation wrappers can be used. You may look at unit tests like this, and follow this pattern of registering a handler with security, and then calling this registered handler to see if it raises aSecurityException
. Both cases of the registered handler passing and failing the security checks should be tested (as done for the validation wrappers). - Would also be useful to check the codecov links to see what parts of your PR have been left uncovered.
1ff1aca
to
3f913a4
Compare
3f913a4
to
256cfff
Compare
d51e2f9
to
a8e4e58
Compare
…allow scopes to be specified in the spec. Having code in place to validate scopes that are disallowed in the spec is pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more minor comments. This is probably the final batch. 🤞
Looks like the python 3.10 e2e is failing after I added the |
@alex-zywicki Adding |
…ensions to requirements.txt
Once this work is in, I will probably follow up with a new issue and PR for per namespace security. I don't want to complicate the core security any more. EDIT: #129 |
@dedoussis Once you're satisfied with the changes I can squash down the commits into a more useful commit message so the git history is less cluttered by this. |
@alex-zywicki I just left a couple of final comments. No need to squash the commits on your branch. Github does it automatically upon merging. Once you resolve the above comments, I will (finally 😅 ) proceed with merging. |
Co-authored-by: Dimitrios Dedoussis <dimitrios@dedouss.is>
I have one small commit coming. Using the SecurityInfo in the test handlers |
This PR does not yet include examples or tests and is intended to be a first draft attempt at #110 and will require further development pending review of initial changes.